-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Replace Rvalue::NullaryOp by a variant in mir::Operand. #148766
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Some changes occurred to the CTFE machinery Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt
cc @rust-lang/wg-const-eval Some changes occurred in cc @BoxyUwU Some changes occurred in src/tools/clippy cc @rust-lang/clippy This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @vakaras Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri This PR changes rustc_public cc @oli-obk, @celinval, @ouz-a Some changes occurred in match checking cc @Nadrieril Some changes occurred to constck cc @fee1-dead This PR changes a file inside |
This comment has been minimized.
This comment has been minimized.
9560745 to
741855c
Compare
This comment has been minimized.
This comment has been minimized.
741855c to
dcf0916
Compare
| /// Special constants whose value depends on the evaluation context. Their value depends on a | ||
| /// flag on the crate being codegenned. | ||
| RuntimeChecks(RuntimeChecks), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For const-eval execution, their value is fixed, right?
|
It seems slightly odd to call these things "constants" when they have a different value inside and outside of const blocks... I am not sure whether this actually breaks anything today, but it seems at least conceptually suboptimal and could lead to issues down the line. For instance, if a constant's body entirely consists just of one of these new constants, then replacing that outer constant by the inner one is not correct (or at least, it can change program behavior) -- so e.g. @saethlin's "trivial const" machinery might need a special case for this. |
|
☔ The latest upstream changes (presumably #148658) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I agree that's not entirely satisfactory. Another design would put the variant inside |
|
That would be marginally better ("values" really should be unchanging), though still a footgun e.g. with the trivial const machinery I mentioned. Making them a new variant of |
dcf0916 to
76bdfae
Compare
This comment has been minimized.
This comment has been minimized.
76bdfae to
68c2a27
Compare
This comment has been minimized.
This comment has been minimized.
Latest commit does that. Cleaner than I expected. |
This comment has been minimized.
This comment has been minimized.
68c2a27 to
4183df4
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| // We can't look at `tcx.sess` here as that can differ across crates, which can lead to | ||
| // unsound differences in evaluating the same constant at different instantiation sites. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment doesn't really make sense in the dummy machine.
| Constant(ref a) => write!(fmt, "{a:?}"), | ||
| Copy(ref place) => write!(fmt, "copy {place:?}"), | ||
| Move(ref place) => write!(fmt, "move {place:?}"), | ||
| RuntimeChecks(checks) => write!(fmt, "const {checks:?}"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"const" is an odd choice, given that we moved it out of the constants
4183df4 to
06de909
Compare
This comment has been minimized.
This comment has been minimized.
e4cc848 to
a02dc34
Compare
It's kind of a wash. If you select "Show non-relevant results" the mean icount result is 0.00%. I'm not worried about this change. |
| // If this is in optimized MIR it's because it's used later, | ||
| // so if we don't need UB checks this session, give a bonus | ||
| // here to offset the cost of the call later. | ||
| self.bonus += CALL_PENALTY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bonus has disappeared now, right? Shouldn't we still add this as the later call is still present?
Cc @scottmcm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure, TBH. Reading this again, it looks like the ubcheck will now get the self.bonus += CONST_SWITCH_BONUS; from the switchInt, like constants do. That's a lower bonus, but we also didn't have the cost from the assignment statement, so 🤷
saethlin's note that this is all messy and adhoc so getting it just right probably needn't block this PR sounds right -- after all, it definitely wasn't just right before either.
I guess in case anyone wants to look at this in the future: this check was an adhoc attempt to remove the inlining penalties from adding ubchecks to standard library methods, many of which the inlining is quite important (NonZero::new_unchecked, for example).
It might be that, after this PR lands, the right thing is to just always treat these checks as false for the purpose of cost calculations, and only follow the successor corresponding to that. That wasn't easy when it needed tracking locals, but now that it's a constant that should be directly in the switchInt, it'd hopefully be fairly straight-forward.
|
@nnethercote Great, thanks for taking a look. :) |
|
@cjgillot was there a reason you removed that, or was that an accident? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me on the changes outside rustc_mir_transform.
For the changes there, in particular the inliner cost logic, I am the wrong reviewer.
r? mir-opt
| } | ||
| // These are essentially constants that didn't end up in an Operand, | ||
| // so treat them as also being free. | ||
| Rvalue::NullaryOp(..) => {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So previously we assigned no cost to assignments with NullaryOp RHS... but we still did assign the full cost to assignments of constants?
And now with the PR we no longer differentiate those cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but we still did assign the full cost to assignments of constants?
Well, SingleUseConsts would remove those assignments entirely in the common cases (particularly the most important if Foo::BAR ones), so it probably didn't much matter overall for inlining.
|
r? saethlin Sorry I missed the attempt to mention me 3 weeks ago, this PR has been quite a storm of notifications and I've been trying to keep up with it but I guess I failed. Some of the perf changes are due to perturbing CGU partitioning. You can tell for example on the very green debug build results for example https://perf.rust-lang.org/detailed-query.html?commit=f642b25044e7b22d14fdca3602c1a4aaf2b036f6&benchmark=eza-0.21.2-debug&scenario=incr-patched:%20printlns&base_commit=7c2c3c0ded2de378bfab2f5b55c387c66fbaf353 if you look at the line for The average impact across all benchmarks is 0.00% so I don't think it makes sense to do a bunch of inline cost estimation tweaking before landing. If someone wants to tweak the cost estimation to optimize perf, they can attempt that very long dev loop outside of a PR that is so conflict-prone. @bors r=RalfJung,saethlin |
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing acfd264 (parent) -> 000ccd6 (this PR) Test differencesShow 38 test diffs38 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 000ccd651d6dfeab13f7703d92a5fd7a9ff7510f --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (000ccd6): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -2.5%, secondary -2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.9%, secondary 1.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.1%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 480.152s -> 481.34s (0.25%) |
Based on #148151
This PR fully removes the MIR
Rvalue::NullaryOp. After #148151, it was only useful for runtime checks likeub_checks,contract_checksandoverflow_checks.These are "runtime" checks, boolean constants that may only be
truein codegen. It depends on a rustc flag passed to codegen, so we need to represent those flags cross-crate.This PR replaces those runtime checks by special variants in MIR
ConstValue. This allows code that expects constants to manipulate those as such, even if we may not always be able to evaluate them to actual scalars.